-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: deleted workspace with invoices is accessible by url #49509
base: main
Are you sure you want to change the base?
Conversation
@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
We have conflicts @gijoe0295 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@gijoe0295 Jest test failing, can you please take a look |
Seems not related to this PR. Will try to merge main soon to see if it's fixed. |
@@ -296,11 +297,11 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, route}: Workspac | |||
const prevProtectedMenuItems = usePrevious(protectedCollectPolicyMenuItems); | |||
const enabledItem = protectedCollectPolicyMenuItems.find((curItem) => !prevProtectedMenuItems.some((prevItem) => curItem.routeName === prevItem.routeName)); | |||
|
|||
const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]); | |||
const prevShouldShowPolicy = usePrevious(shouldShowPolicy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you do it like this it seems to be not working sometimes and sometime i still see workspacepages we want something like mentioned in this proposal #49093 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see in this vid. workspace is deleted but it still navigates to the page
Screen.Recording.2024-09-27.at.12.20.38.AM-1.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but for me your solution works only sometimes, mainly on small screens it often does not work @gijoe0295
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ishpaul777 Besides the above issue, I pushed the approach mentioned in your linked proposal.
Screen.Recording.2024-09-28.at.01.43.58.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay thanks! i'll test again in my morning, off for the day today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gijoe0295, thanks for reaching out. Unfortunately, I no longer work for Expensify. If you need help with this issue, you can try asking C+, who was responsible for this issue, or someone else who participated in the discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops thank you. Sorry for the inconvenience.
I am having trouble sending invoice to any user, it was working last i tested @gijoe0295 can you please check on your side, Screen.Recording.2024-09-25.at.11.51.39.PM.mov |
Worked normally for me after merging latest |
Bug: [IOS] Infinite loading when navigating to deleted workspace page Screen.Recording.2024-10-01.at.1.56.52.AM.mov |
gentle bump. @gijoe0295 ^ |
@gijoe0295 Its already 3 days without any response, Are you still interested finishing this PR? |
Sorry. Still taking a look. Will provide updates in several hours. |
@ishpaul777 Can you please retry or provide reproduction steps? I retried many times but still couldn't reproduce the infinite loading. |
i'll give this a test again on monday this was consistently reproducable for me last i checked. Steps are same as in PR description,only this different, i was not able to navigate using the link in message (it pointing to staging) so i copied dev. link) |
still reproducable, steps same as in PR test steps Untitled.mov |
Details
Fixed Issues
$ #49093
PROPOSAL: #49093 (comment)
Tests
Precondition: User B has no workspace
Offline tests
NA
QA Steps
Precondition: User B has no workspace
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-09-20.at.10.12.21.mov
Android: mWeb Chrome
Screen.Recording.2024-09-20.at.10.09.41.mov
iOS: Native
Screen.Recording.2024-09-20.at.10.02.41.mov
iOS: mWeb Safari
Screen.Recording.2024-09-20.at.10.05.46.mov
MacOS: Chrome / Safari
Screen.Recording.2024-09-20.at.09.51.20.mov
MacOS: Desktop
Screen.Recording.2024-09-20.at.10.06.52.mov